Fix ACL constraint error (return constraint error instead of failure when the ACL entry is invalid) #20736
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
erjiaqing,
franck-apple,
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
jtung-apple,
kghost,
kpschoedel,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns and
msandstedt
… and seems like a whack-a-mole bug fixing
…ayering purposes. Layering still not ideal though.
woody-apple
force-pushed
the
acl_constraints
branch
from
7f3a6d3 to
fde974e
Compare
|
PR #20736: Size comparison from 8a32b6c to fde974e Increases (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (41 builds for cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
|
PR #20736: Size comparison from e4cfa0a to bbc438d Increases (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
…ar why it fails in CI and it does NOT fail locally
|
PR #20736: Size comparison from e4cfa0a to aced054 Increases (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Decreases (6 builds for cc13x2_26x2)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
github-actions bot
pushed a commit
that referenced
this pull request
Jul 18, 2022
…when the ACL entry is invalid) (#20736) * Define a test for access control constraints * Typgo fixes * Restyle * Fix some text and formatting * Zap regen * Make ACL cluster return constraint error if the ACL entry is not valid * Add one more test: Step 31 * Zap regen for the new test * Added the rest of the tests from #20672 * Restyle * Removed some steps: too large numbers for subjects, cannot be represented * Zap regen * Restyle * Fix build, add more ACL changes, zap regen after adjusting test case to match bug report * Fix more things to return CONSTRAINT_ERROR * Convert more invalid argument to constraint errors. This is not ideal and seems like a whack-a-mole bug fixing * Restyle * Fix comments in yaml * Restyle * Restyle messed things up. Corrected it * One more comment fix * Restyle * Split out IM status code header and cpp into a separate library for layering purposes. Layering still not ideal though. * Restyle * Also update TestAccessControlCluster * Zap regen * Updated test ACL error codes a bit and zap regen * Update logic to centrailize error code processing location * Added unit test for step 35 as well (pass) * Added even more tests and updated formatting of ACL a bit for readability * Restyle * One more test for invalid privilege * Restyle
woody-apple
added a commit
that referenced
this pull request
Jul 18, 2022
…when the ACL entry is invalid) (#20736) (#20890) * Define a test for access control constraints * Typgo fixes * Restyle * Fix some text and formatting * Zap regen * Make ACL cluster return constraint error if the ACL entry is not valid * Add one more test: Step 31 * Zap regen for the new test * Added the rest of the tests from #20672 * Restyle * Removed some steps: too large numbers for subjects, cannot be represented * Zap regen * Restyle * Fix build, add more ACL changes, zap regen after adjusting test case to match bug report * Fix more things to return CONSTRAINT_ERROR * Convert more invalid argument to constraint errors. This is not ideal and seems like a whack-a-mole bug fixing * Restyle * Fix comments in yaml * Restyle * Restyle messed things up. Corrected it * One more comment fix * Restyle * Split out IM status code header and cpp into a separate library for layering purposes. Layering still not ideal though. * Restyle * Also update TestAccessControlCluster * Zap regen * Updated test ACL error codes a bit and zap regen * Update logic to centrailize error code processing location * Added unit test for step 35 as well (pass) * Added even more tests and updated formatting of ACL a bit for readability * Restyle * One more test for invalid privilege * Restyle Co-authored-by: Andrei Litvin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
FAILUREreported instead ofCONSTRAINT_ERRORFixes #20672
Fixes #20794
Fixes #20796
Fixes #20797 (NOTE: uses 0xFFFF_FFFD_0000_0000 and NOT the original test value as that seemed odd)
Fixes #20798
Change overview
Fix return code
Add unit tests for automation
I could NOT add tests for the 64bit invalid subjects due to zap limitation (was later explained that I need to use quotes). In the mean time, the tests not covered were broken out in separate bugs that will be fixed separately.
Testing
Automated testing (yay!)